feat: adding gh changelog/releases to npmx#1233
Conversation
…md -> html does need to change)
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
excludes changelog releases due to needing api calls
|
left one minor comment but this generally looks good to me. i think we need to move this on as its been continuously reviewed since Feb. what'd be helpful:
|
|
for further significant changes I've already made a new branch (compare pr), the pr will be made after this pr gets merged. I do hope that this pr can be merged soon because there are further plans. |
43081j
left a comment
There was a problem hiding this comment.
approving but please hold off merging until we get another approval as this is a big one.
|
Yeah the skeletons loaders can be improved, currently they're only being shown when fetching data but not during server rendering of the changelog components due to not accounting for ssr. |
|
@ghostdevv @serhalp @alexdln could you re-review this please, thanks! |
|
(not blockers, we should merge and follow up!)
npmx.pr.1233.bug.1.mp4
Thank you so much for all your work on this! It's been a hell of a long road. 🙏🏼 This is going to be a killer feature! |
There was a problem hiding this comment.
Are these because we don't have a working strategy yet for i18n strings in server → browser error messages? 🤔
There was a problem hiding this comment.
yes, these were even made before the discussion of having i18n strings on the server side.
| 'Changelog/Releases.vue': 'Requires API calls', | ||
| 'Changelog/Markdown.vue': 'Requires API call & only renders markdown html', |
There was a problem hiding this comment.
requiring API calls isn't strictly a blocker here, is it? many components do 🤔
There was a problem hiding this comment.
apart from requiring api calls, both components do also mostly render html from the markdown renderer.
so an a11y test would mostly test things that the components doesn't have influence over.
if you know a way to test these components, I'm open to implement the tests
| if (!release.publishedAt) { | ||
| return | ||
| } | ||
| return requestedDate === toIsoDate(new Date(release.publishedAt)) |
There was a problem hiding this comment.
nit: is this redundant or am I missing something? 🤔
| return requestedDate === toIsoDate(new Date(release.publishedAt)) | |
| return requestedDate === release.publishedAt |
There was a problem hiding this comment.
the string -> date -> string isn't needed, I've now implemented that the date is now sliced out of the full iso date+time string
| const uChangelog = changelog.value | ||
| if (uChangelog?.type === 'md' || uChangelog?.type === 'release') { |
There was a problem hiding this comment.
nit: this seems smelly to me? seems like there should be some sort of abstraction here like shouldShowChangelog or hasValidChangelog or whatever
There was a problem hiding this comment.
I've just moved this to composables for both packageHeader & command palette.
for command palette it does use useState because of the buggy behavior I commented on earlier, the state is provided by the package header component
WilcoSp@e3bafc5
I've looked into what causes "no changelog available". for the not navigating I think it's partly caused by ^^ because when no package name is known yet currently "data;text/json,null" is given as url to return null, but the mime type should be "application" instead of "text".
yes I know about it, for now all releases are shown unfiltered but I do want to filter these in the future for monorepos, only issue is that everyone does it a bit different, some
if you let me know how, I'll be happy to add it. |
|
@WilcoSp Thank you for all your work and persistence on this! I know it's been a long road. I'm going to merge it now. It will be on main.npmx.dev, where we can iterate on the follow-ups! 🚀 |
This pr will add the possibility to view the changelog.md & releases from a package's github repo within npmx.
This will make it easier to see the changelogs while not needing to leave npmx and allowing quicker access.
This pr is the first pr of #501
Preview here, this will automatically update
Acknowledgements
While I was making this PR antfu also made this pr #1368 and here my comment from his pr
Also @ShroXd is currently working on adding changelogs to the version history page that was merged with #2025, we've both agreed to first independently work on our PRs and later after both are merged and we've received feedback & irl use we'll collaborate on the interaction between the two
Translations
Thanks @WarningImHack3r for translating the text for changelog to French
I've also added Dutch translations